refactor: remove -isystem on local include header files #211
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the codebase to remove the -isystem flag from local include header files and moves the TransformTypeToString function implementation from the source file to the header file. The change affects how include directories are treated during compilation and consolidates transform type string conversion logic.
- Moves
TransformTypeToStringfunction and string constants from.ccfile to.hfile - Removes
SYSTEMflag from CMake include directory configurations - Eliminates anonymous namespace usage for transform type constants
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/iceberg/transform.h | Adds string constants and inline implementation of TransformTypeToString function |
| src/iceberg/transform.cc | Removes string constants and TransformTypeToString function implementation |
| cmake_modules/IcebergBuildUtils.cmake | Removes SYSTEM flag from target_include_directories calls |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
HuaHuaY
force-pushed
the
remove_wrong_access
branch
from
54f1f6b to
5b734f6
Compare
| /// The highest assigned column ID for the table | ||
| int32_t last_column_id; | ||
| /// A list of schemas | ||
| std::vector<std::shared_ptr<Schema>> schemas; |
There was a problem hiding this comment.
I don't think this is necessary since we are inside a iceberg namespace here, same for the below.
There was a problem hiding this comment.
If we don't add iceberg::, gcc will report a compile error due to -Werror and -Wchanges-meaning (included in -Wall).
There was a problem hiding this comment.
https://github.com/apache/iceberg-cpp/actions/runs/17463347891/job/49593031029?pr=211
In file included from /home/runner/work/iceberg-cpp/iceberg-cpp/src/iceberg/catalog/in_memory_catalog.cc:27:
/home/runner/work/iceberg-cpp/iceberg-cpp/src/iceberg/table_metadata.h:125:35: error: declaration of ‘iceberg::Result<std::shared_ptr<iceberg::Schema> > iceberg::TableMetadata::Schema() const’ changes meaning of ‘Schema’ [-Wchanges-meaning]
125 | Result<std::shared_ptr<Schema>> Schema() const;
| ^~~~~~
|
I use clang in macos for coding but ci uses gcc. The two environments have some different compile flags in |
HuaHuaY
force-pushed
the
remove_wrong_access
branch
from
c4492a1 to
eff4435
Compare
|
|
||
| /// \brief The codec type of the table metadata file. | ||
| ICEBERG_EXPORT enum class MetadataFileCodecType { | ||
| enum class ICEBERG_EXPORT MetadataFileCodecType { |
There was a problem hiding this comment.
https://github.com/apache/iceberg-cpp/actions/runs/17463347891/job/49593031039
In file included from /home/runner/work/iceberg-cpp/iceberg-cpp/src/iceberg/catalog/in_memory_catalog.cc:27:
/home/runner/work/iceberg-cpp/iceberg-cpp/src/iceberg/table_metadata.h:148:27: error: attribute ignored in declaration of ‘enum class iceberg::MetadataFileCodecType’ [-Werror=attributes]
148 | ICEBERG_EXPORT enum class MetadataFileCodecType {
| ^~~~~~~~~~~~~~~~~~~~~
/home/runner/work/iceberg-cpp/iceberg-cpp/src/iceberg/table_metadata.h:148:27: note: attribute for ‘enum class iceberg::MetadataFileCodecType’ must follow the ‘enum class’ keyword
HuaHuaY
force-pushed
the
remove_wrong_access
branch
from
805adf9 to
bfb1d72
Compare
| endif() | ||
|
|
||
| if(LIB_INCLUDES) | ||
| target_include_directories(${LIB_NAME}_shared SYSTEM PUBLIC ${ARG_EXTRA_INCLUDES}) |
There was a problem hiding this comment.
@raulcd I think I just copied the same thing from Apache Arrow. Do you have any idea why it was added there?
There was a problem hiding this comment.
I read arrow's cmake files. It doesn't use the parameter EXTRA_INCLUDES of add_arrow_lib. So maybe we misunderstand the meaning of EXTRA_INCLUDES and we should modify the caller of add_iceberg_lib instead of here.
There was a problem hiding this comment.
Adding these header files as build time dependencies makes it easier for libiceberg-bundle and unit test executables to find them. But I also think we don't need to add SYSTEM to them in this case.
There was a problem hiding this comment.
This was added ~7 years ago here:
apache/arrow@5c97cd6
I also think SYSTEM is not required, we could try removing it on ARROW too and validate CI, Python bindings builds, etcetera.
HuaHuaY
force-pushed
the
remove_wrong_access
branch
from
bfb1d72 to
5ff94da
Compare
HuaHuaY
force-pushed
the
remove_wrong_access
branch
from
5ff94da to
870dc49
Compare
HuaHuaY
force-pushed
the
remove_wrong_access
branch
from
870dc49 to
988d8b1
Compare
6 participants
No description provided.